Add support for table valued functions for SQL Server #1839

aharpervc · 2025-05-06T19:22:57Z

This PR adds support for table valued functions for SQL Server, both inline & multi statement functions. For reference, that's the B & C documentation here: https://learn.microsoft.com/en-us/sql/t-sql/statements/create-function-transact-sql?view=sql-server-ver16#b-create-an-inline-table-valued-function

Inline TVF's are defined with AS RETURN, so we have a new CreateFunctionBody::AsReturn variant accordingly. Functions using "AS RETURN" don't have BEGIN/END, so that part of the parsing logic is now conditional. Additionally, the data type parser now supports "RETURNS TABLE" without a table definition.

Multi statement TVF's use named table expressions, so a new NamedTable data type variant was added. I didn't see a great way to integrate this into the existing data type parser (especially without rewinding), so creating this data type happens inside the parse create function logic first by parsing the identifier, then parsing the table definition, then using those elements to produce a NamedTable.

I also added a new test example for each of these scenarios.

aharpervc · 2025-05-07T16:26:19Z

tests/sqlparser_mssql.rs

+        CREATE FUNCTION some_inline_tvf(@foo INT, @bar VARCHAR(256)) \
+        RETURNS TABLE \
+        AS \
+        RETURN (SELECT 1 AS col_1)\


Parentheses are optional for inline tvf return queries, although I think the subquery expr expects/requires them currently.

I added support for that syntax & added a new test case example

UNION is also supported in this syntax but not in this current approach due to using parse_select, which is restricted. I'm comfortable leaving that for later

iffyio

took a quick look and left a couple comments, @aharpervc could you rebase on main now that the other PR has landed, in order to remove the extra diff?

iffyio · 2025-05-08T23:50:52Z

src/parser/mod.rs

+        if self.peek_keyword(Keyword::AS) {
+            self.expect_keyword_is(Keyword::AS)?;
+        }


Suggested change

if self.peek_keyword(Keyword::AS) {

self.expect_keyword_is(Keyword::AS)?;

}

self.parse_keyword(Keyword::AS);

this looks like it could be simplified as above?

iffyio · 2025-05-08T23:52:36Z

src/ast/data_type.rs

+                if fields.is_empty() {
+                    return write!(f, "TABLE");
+                }
+                write!(f, "TABLE({})", display_comma_separated(fields))


I think instead of using the is_empty we can make the fields value an Option and skip the parenthesis only if fields is None. Otherwise we could run into issues if empty fields is allowed in the other variant of this datatype

iffyio · 2025-05-08T23:53:35Z

src/parser/mod.rs

+        let return_type = if return_table.is_some() {
+            return_table
+        } else {
+            Some(self.parse_data_type()?)
+        };


Could this be included in the maybe_parse() logic above? such that the logic returns both the return table and the datatype. based on this condition, they seem to go hand in hand?

Hm, I don't see how that would be done. The use of maybe_parse is to get the table name & other parts, but if we don't have that then we should rewind and just parse a typical data type. Do you mean like having another maybe_parse inside the first maybe_parse?

Ah indeed, in this case its one or the other, so should be fine to leave as is.

- rename `AsReturn` to `AsReturnSubquery` for clarity between these two variants

aharpervc · 2025-05-09T15:12:45Z

took a quick look and left a couple comments, @aharpervc could you rebase on main now that the other PR has landed, in order to remove the extra diff?

Done 👍

iffyio · 2025-05-10T00:50:23Z

src/ast/data_type.rs

+    NamedTable(
+        /// Table name.
+        ObjectName,
+        /// Table columns.
+        Vec<ColumnDef>,
+    ),


Suggested change

NamedTable(

/// Table name.

ObjectName,

/// Table columns.

Vec<ColumnDef>,

),

NamedTable {

/// Table name.

table: ObjectName,

/// Table columns.

columns: Vec<ColumnDef>,

},

we can use an anonymous struct in this manner?

Oops, yes we can. Not sure why I did it that way. Done 👍

iffyio · 2025-05-10T00:50:52Z

src/ast/data_type.rs

-    Table(Vec<ColumnDef>),
+    /// [MsSQL]: https://learn.microsoft.com/en-us/sql/t-sql/statements/create-function-transact-sql?view=sql-server-ver16#c-create-a-multi-statement-table-valued-function
+    Table(Option<Vec<ColumnDef>>),
+    /// Table type with a name, e.g. CREATE FUNCTION RETURNS @result TABLE(...).


Could we add a link to the docs that support NamedTable variant?

iffyio · 2025-05-10T00:51:36Z

src/ast/mod.rs

+    /// RETURNS TABLE
+    /// AS RETURN SELECT a + b AS sum;
+    /// ```
+    AsReturnSelect(Select),


maybe we can also include a reference doc link here?

iffyio · 2025-05-10T00:52:58Z

src/parser/mod.rs

-        }));
+            Ok(DataType::NamedTable(
+                ObjectName(vec![ObjectNamePart::Identifier(return_table_name)]),
+                table_column_defs.clone().unwrap(),


can we return an error instead of the unwrap here?

iffyio · 2025-05-10T00:54:02Z

src/parser/mod.rs

-        self.expect_keyword_is(Keyword::AS)?;
+        let return_table = self.maybe_parse(|p| {
+            let return_table_name = p.parse_identifier()?;
+            let table_column_defs = if p.peek_keyword(Keyword::TABLE) {


I think we can replace the if/else statement here with self.expect_keyword(Keyword::TABLE)

iffyio · 2025-05-10T00:54:42Z

src/parser/mod.rs

-        let statements = self.parse_statement_list(&[Keyword::END])?;
-        let end_token = self.expect_keyword(Keyword::END)?;
+            if table_column_defs.is_none()
+                || table_column_defs.clone().is_some_and(|tcd| tcd.is_empty())


hmm is the clone() here necessary?

iffyio · 2025-05-10T00:57:26Z

src/parser/mod.rs

+            let return_table_name = p.parse_identifier()?;
+            let table_column_defs = if p.peek_keyword(Keyword::TABLE) {
+                match p.parse_data_type()? {
+                    DataType::Table(t) => t,


I think already here we can check that the returned data type is none empty, that would avoid the option invalid case propagating further below (where we have the is_none and is_some checks). e.g.

DataType::Table(Some(t)) if !t.is_empty() => t

iffyio · 2025-05-10T00:57:49Z

src/parser/mod.rs

-        }));
+            Ok(DataType::NamedTable(
+                ObjectName(vec![ObjectNamePart::Identifier(return_table_name)]),
+                table_column_defs.clone().unwrap(),


similarly here, is the clone necessary?

iffyio · 2025-05-10T01:00:36Z

src/parser/mod.rs

+        let return_type = if return_table.is_some() {
+            return_table
+        } else {
+            Some(self.parse_data_type()?)
+        };


Ah indeed, in this case its one or the other, so should be fine to leave as is.

iffyio · 2025-05-11T00:08:38Z

src/parser/mod.rs

+                if !matches!(expr, Expr::Subquery(_)) {
+                    parser_err!(
+                        "Expected a subquery after RETURN",
+                        self.peek_token().span.start
+                    )?
+                }
+                Some(CreateFunctionBody::AsReturnSubquery(expr))


Suggested change

if !matches!(expr, Expr::Subquery(_)) {

parser_err!(

"Expected a subquery after RETURN",

self.peek_token().span.start

)?

}

Some(CreateFunctionBody::AsReturnSubquery(expr))

Some(CreateFunctionBody::AsReturnExpr(expr))

thinking since a subquery is already an expr, we can be more permissive here?

I think this is a "could" vs "should" situation. The question here is what is the parser expecting? I don't think it makes sense to parse arbitrary expr's if they wouldn't otherwise be allowed by any sql engine... that seems better as a parse failure.

Yeah ideally we would parse statements accordingy to spec but the parser is more permissive in some cases, especially in order to minimize complexity in the parser. In this case we would avoid introducing an extra AsReturnSubquery enum variant, as well as the matches!() check that does the validation after the fact of parsing an expression which isn't ideal. So I think we'd be better off leaving the validation to the downstream crate in this scenario

Hm, yes I suppose that makes sense. It seems odd for the parser to produce output if it's known to be invalid, but I suppose you're saying, the validity question is in the semantics rather than the syntax.

Loosening the implementation to any Expr would probably mean that select .. union .. select syntax would begin parsing, which addresses my concern above.

I'll investigate...

iffyio · 2025-05-14T07:28:12Z

src/parser/mod.rs

+                    if self.peek_token() != Token::LParen {
+                        Ok(DataType::Table(None))


can we add a comment here explaining what the LParen check is for?

iffyio · 2025-05-14T07:32:38Z

src/parser/mod.rs

+                if !matches!(expr, Expr::Subquery(_)) {
+                    parser_err!(
+                        "Expected a subquery after RETURN",
+                        self.peek_token().span.start
+                    )?
+                }
+                Some(CreateFunctionBody::AsReturnSubquery(expr))


Yeah ideally we would parse statements accordingy to spec but the parser is more permissive in some cases, especially in order to minimize complexity in the parser. In this case we would avoid introducing an extra AsReturnSubquery enum variant, as well as the matches!() check that does the validation after the fact of parsing an expression which isn't ideal. So I think we'd be better off leaving the validation to the downstream crate in this scenario

aharpervc marked this pull request as ready for review May 6, 2025 19:24

aharpervc changed the title ~~Add support for table valued functions for SQL Serve~~ Add support for table valued functions for SQL Server May 6, 2025

aharpervc force-pushed the mssql-create-tvf branch 3 times, most recently from 3686b1b to db1c9b2 Compare May 6, 2025 23:37

aharpervc commented May 7, 2025

View reviewed changes

iffyio reviewed May 8, 2025

View reviewed changes

aharpervc added 8 commits May 9, 2025 10:54

Add support for inline table valued functions for SQL Server

54b2829

Add multi-statement table valued function support for SQL Server

4793aeb

Enable parsing CREATE FUNCTION without AS

9f08de0

Add support for constraints in table valued function definitions

a54aed6

Corrected syntax formatting

6346ecc

Add support for un-parenthesized "RETURN SELECT" syntax

a27c22c

- rename `AsReturn` to `AsReturnSubquery` for clarity between these two variants

Simplify peek/expect with parse_keyword

95f1bc6

Make Table's ColumnDefs optional

999964c

aharpervc force-pushed the mssql-create-tvf branch from 1b92ede to 999964c Compare May 9, 2025 15:10

aharpervc requested a review from iffyio May 9, 2025 15:12

iffyio reviewed May 10, 2025

View reviewed changes

iffyio reviewed May 11, 2025

View reviewed changes

aharpervc added 4 commits May 13, 2025 12:17

Refactor to avoid cloning/unwrapping

764e28d

Refactor NamedTable to be a regular struct

7943c16

Add documentation link for named tables types

9aa7ae6

Add documentation link for RETURN SELECT

91b2db8

iffyio reviewed May 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for table valued functions for SQL Server #1839

Add support for table valued functions for SQL Server #1839

aharpervc commented May 6, 2025 •

edited

Loading

aharpervc May 7, 2025

aharpervc May 7, 2025

aharpervc May 7, 2025 •

edited

Loading

iffyio left a comment

iffyio May 8, 2025

aharpervc May 9, 2025

iffyio May 8, 2025

aharpervc May 9, 2025

iffyio May 8, 2025

aharpervc May 9, 2025

iffyio May 10, 2025

aharpervc commented May 9, 2025

iffyio May 10, 2025

aharpervc May 13, 2025

iffyio May 10, 2025

aharpervc May 13, 2025

iffyio May 10, 2025

aharpervc May 13, 2025

iffyio May 10, 2025

iffyio May 10, 2025

iffyio May 10, 2025

iffyio May 10, 2025

iffyio May 10, 2025

iffyio May 10, 2025

iffyio May 11, 2025

aharpervc May 13, 2025

iffyio May 14, 2025

aharpervc May 14, 2025

iffyio May 14, 2025

iffyio May 14, 2025

		if self.peek_token() != Token::LParen {
		Ok(DataType::Table(None))

Add support for table valued functions for SQL Server #1839

Are you sure you want to change the base?

Add support for table valued functions for SQL Server #1839

Conversation

aharpervc commented May 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aharpervc May 7, 2025 • edited Loading

Choose a reason for hiding this comment

iffyio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aharpervc commented May 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aharpervc commented May 6, 2025 •

edited

Loading

aharpervc May 7, 2025 •

edited

Loading